- 
                Notifications
    You must be signed in to change notification settings 
- Fork 5.1k
test: enable parameterized test for HttpProtocolIntegrationTest #41635
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Christoph Pakulski <[email protected]>
Signed-off-by: Christoph Pakulski <[email protected]>
Signed-off-by: Christoph Pakulski <[email protected]>
Signed-off-by: Christoph Pakulski <[email protected]>
Signed-off-by: Christoph Pakulski <[email protected]>
Signed-off-by: Christoph Pakulski <[email protected]>
| @ravenblackx IIRC you are maintainer for tests. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having argued for not using TEST_P for this example case, I'm not sure if there is a realistic case for where you'd really want to combine another parameterization on top of the IPv4/IPv6/Http1/Http2/Http3 combo that the integration tests already use.
I don't fundamentally object to making it possible to do so, but I would very much like to discourage doing so when it's not actually appropriate. We already have a bunch of TEST_P that shouldn't be.
| : public HttpProtocolIntegrationTestWithParams<std::tuple< | ||
| std::string, absl::string_view, absl::string_view, uint32_t, absl::string_view>> { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make this a struct rather than a tuple with explanatory comments and const indices.
Or, better for this purpose, don't use parameterized testing for this at all - it's much clearer to just have member variables in the test class, so instead of something like
TEST_P(OutlierDetectionIntegrationTest, TestEverything) {
  auto x = GetParam(1).x;
  auto y = GetParam(1).y;
  auto foo = initializeXY(x, y);
  auto expected = GetParam(1).expected;
  EXPECT_THAT(doStuff(foo), Eq(expected));
}
INSTANTIATE_TEST_SUITE_P(
  TestArgStruct{"1And2ResultsIn3", 1, 2, 3},
  TestArgStruct{"4And5ResultsIn9", 4, 5, 9},
);
you just do
TEST_F(OutlierDetectionIntegrationTest, Test1and2ResultsIn3) {
  auto foo = initializeXY(1, 2);
  EXPECT_THAT(doStuff(foo), Eq(3));
}
TEST_F(OutlierDetectionIntegrationTest, Test4and5ResultsIn9) {
  auto foo = initializeXY(4, 5);
  EXPECT_THAT(doStuff(foo), Eq(9));
}
Even though if there are many tests this may result in more lines (it typically doesn't actually because the fields usually end up one line each, and passing the same values into helper functions is also usually one line), it makes for test code that's dramatically easier to read and understand, and you don't need to include special stringification functions and struct/tuple definitions etc.
Helper functions in the test class, member variables in the test class for persistent stuff, and custom matchers, will typically be fewer lines overall than a monolithic "do all the same stuff with different inputs" function - you can still put all the repetitive stuff inside helper functions.
In general if you use TEST_P with only one test, you shouldn't be using TEST_P - it's useful for the combinatorial thing where you want to run multiple tests with a range of configuration states.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ravenblackx Thanks for reviewing. I think you have very good points here. What bothered me was the amount of repeated code and I was under the impression that the best way to solve them was using parameterized tests. But your point that it is difficult to read such tests is a very valid one! Especially, when COMBINE is used to built a matrix of parameters.
I am going to close this PR and refactor my tests to use helper functions (as you suggest), which should be much cleaner and easier to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more minor suggestion that may or may not help - sometimes a good thing to do is subclass a test class with some common initialization, so you can have a "generic" test class you use for awkward cases, where you call the helper functions explicitly, and the "common" test class where all the initialization and teardown helpers are called with default args in the SetUp/TearDown/constructor/destructor of the class and you just do the one differing part explicitly.
(But also sometimes it may be clearer to just always explicitly call the helpers, or use a short version of the helpers like defaultInitialize().)
The subclass thing is especially likely to be useful in integration tests because there sometimes the integration test superclass performs some initialization, so you have to prepare different setup stuff during the constructor or SetUp function, via an overridden virtual function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The subclass thing is especially likely to be useful in integration tests because there sometimes the integration test superclass performs some initialization, so you have to prepare different setup stuff during the constructor or SetUp function, via an overridden virtual function.
This is a great point. I will try to refactor the same tests I tried to parameterize in this PR and will see where special initialization is needed.
| Closing as better alternatives were found. | 
Commit Message:
enable parameterized tests for
HttpProtocolIntegrationTestAdditional Description:
While working on #39947, I noticed that it was impossible to write parameterized tests for classes derived from
HttpProtocolIntegrationTest, becauseHttpProtocolIntegrationTestwas derived fromtesting::TestWithParam<HttpProtocolTestParams>, so adding new params was not possible. This resulted in lots of duplicated test routines with little differences. This PR reshuffles test code/classes to allow adding new params on top ofHttpProtocolTestParams, but leaves existing routines derived fromHttpProtocolIntegrationTestintact. All tests which I added in #39947 were parameterized.Risk Level: Low
Testing: All existing tests should pass
Docs Changes: No
Release Notes: No
Platform Specific Features: